Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BT-684 Initial Blob Storage Impl #6810

Merged
merged 8 commits into from
Jul 26, 2022
Merged

BT-684 Initial Blob Storage Impl #6810

merged 8 commits into from
Jul 26, 2022

Conversation

kraefrei
Copy link
Contributor

No description provided.

project/Dependencies.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

@jgainerdewar jgainerdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a first pass of review, this looks great!! I'll try to come back later and look more closely for Scala style stuff as well.

@kraefrei kraefrei marked this pull request as ready for review July 22, 2022 13:01
Copy link
Collaborator

@jgainerdewar jgainerdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'm sure we'll continue tweaking this over time, no reason not to go ahead and merge v1!

@jgainerdewar
Copy link
Collaborator

Should be OK to go ahead and merge without the code coverage approval - it reflects the tests that actually ran on this PR, so it's finding none. If there's a way to only ignore the test we expect to fail, might be nice, but not a blocker for this PR IMO.

* pathToFile -> test/testFile.wdl
*
* If the configured container and storage account do not match, the string is considered unparsable
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice explanation.

Copy link
Contributor

@mspector mspector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kraefrei kraefrei merged commit 95c550e into develop Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants